Skip to content

feat(api): ensure StackRunConfig #3543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat(api): ensure StackRunConfig #3543

wants to merge 1 commit into from

Conversation

@cdoern
Copy link
Contributor

@cdoern cdoern commented Sep 24, 2025 •
edited
Loading

What does this PR do?

StackRunConfig is part of our public API, ensure stability of this datatype using a pytest snapshot test.

If the pydantic model changes, it will fail. A snapshot can be re-generated via @github-actions regenerate snapshots by a code owner.

The API conformance test will then re-run and pass.

added json_schema_extra to two spots in the run config so user paths do not get propagated into the snapshot json

Test Plan

API Conformance Test should pass.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 24, 2025
@cdoern
Copy link
Contributor Author

cdoern commented Sep 24, 2025

working on this -- might convert to live in tests/

@cdoern cdoern force-pushed the run-validation branch 7 times, most recently from 59a098a to 90a9e9c Compare October 2, 2025 20:04
@cdoern cdoern marked this pull request as ready for review October 2, 2025 20:10
@cdoern
Copy link
Contributor Author

cdoern commented Oct 2, 2025

uh oh, pre commit seems broken still?

leseb
leseb requested changes Oct 3, 2025
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the approach. I have a few questions:

  • how do we intend to use this? Do we bump the version each time a new change to the schema is introduced?
  • How do we refresh the tests/api/snapshots/test_models/test_run_config_v1_schema_is_unchanged/stored_run_config_v1_schema.json file? Should pre-commit do it? Should we run the scripts/api-conformance.sh script with a special flag to rehydrate?

I think we need some guidelines, not just a script that fails the CI :) (unless I missed something!)

Thanks!

scripts/snapshot-test.sh
@@ -0,0 +1,25 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a more accurate name for the script?

@cdoern cdoern force-pushed the run-validation branch 2 times, most recently from 2c3c461 to f694a21 Compare October 7, 2025 19:55
@cdoern
Copy link
Contributor Author

cdoern commented Oct 7, 2025

@leseb , I made it so that when a breaking change is acknowledged through the proper process ! or BREAKING CHANGE:, the pydantic test is run and always regenerates the schema and does not fail.

Also changed the name of the script to test_pydantic_models

@cdoern cdoern force-pushed the run-validation branch 2 times, most recently from 2c65dee to 0cacb3d Compare October 8, 2025 14:05
@cdoern
Copy link
Contributor Author

cdoern commented Oct 8, 2025

FYI I need to rebase this each time the run config changes, sorry for the churn :)

leseb
leseb reviewed Oct 9, 2025
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove tests/api/snapshots/test_models/test_run_config_v1_schema_is_unchanged/stored_run_config_v1_schema.json?

.github/workflows/conformance.yml
oasdiff breaking --fail-on ERR $BASE_SPEC $CURRENT_SPEC --match-path '^/v1/'
# never skip this, instead if a breaking change is properly identified -- we should regenerate our snapshot.
- name: Run Pydantic Model Test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we run this in pre-commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keeping it in the conformance yml makes sense though so it can get the context of the ! or BREAKING CHANGE: from this workflow. In pre-commit people will just override the snapshots locally and repush.

scripts/api-conformance.sh Outdated
Comment on lines 59 to 63
pytest -s -v "$SCRIPT" --snapshot-update
# When regenerating, always exit 0 to make the test pass
exit 0
else
pytest -s -v "$SCRIPT" --snapshot-update
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same command regardlesss if REGENERATE_SNAPSHOT is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made this:

# Run pytest with snapshot testing
echo "=== Running Snapshot Tests ==="
  if [[ "$REGENERATE_SNAPSHOT" == "true" ]]; then
    echo "Regenerating snapshots..."
    pytest -s -v "$SCRIPT" --snapshot-update
    exit 0
  else
    pytest -s -v "$SCRIPT"  # do not update snapshots.
  fi

so that the test only updates the snapshots and passes if the proper ack is given. The exit 0 is necessary because even when updating snapshots these types of tests fail on the diff I think.

the other way will fail if the snapshots differ.

@cdoern cdoern force-pushed the run-validation branch 3 times, most recently from cf7abc7 to b86766d Compare October 10, 2025 19:17
cdoern
cdoern commented Oct 10, 2025
View reviewed changes
.github/workflows/bot-trigger.yml
@@ -0,0 +1,368 @@
name: PR Bot Commands
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a re-written version of precommit-trigger. I used git mv but because the diff is so large it's registering it as a new file annoyingly. Sorry

@cdoern cdoern force-pushed the run-validation branch 2 times, most recently from b62a28a to 680cfde Compare October 10, 2025 19:38
@cdoern cdoern marked this pull request as draft October 13, 2025 02:25
@cdoern
Copy link
Contributor Author

cdoern commented Oct 13, 2025

moving to draft while I wrestle with the new action I am introducing.

@cdoern
feat(api): ensure StackRunConfig
af94606 
StackRunConfig is part of our public API, ensure stability of this datatype using a pytest snapshot test.

If the pydantic model changes, it will fail. A snapshot can be re-generated via `@github-actions regenerate snapshots` by a code owner.

The API conformance test will then re-run and pass.

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@cdoern cdoern mentioned this pull request Oct 28, 2025
Merged
@cdoern
Copy link
Contributor Author

cdoern commented Oct 29, 2025

closing in favor of #3952

@cdoern cdoern closed this Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leseb leseb leseb requested changes

@ashwinb ashwinb Awaiting requested review from ashwinb ashwinb is a code owner

@yanxi0830 yanxi0830 Awaiting requested review from yanxi0830 yanxi0830 is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah is a code owner

@raghotham raghotham Awaiting requested review from raghotham raghotham is a code owner

@ehhuang ehhuang Awaiting requested review from ehhuang ehhuang is a code owner

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan terrytangyuan is a code owner

@bbrowning bbrowning Awaiting requested review from bbrowning bbrowning is a code owner

@reluctantfuturist reluctantfuturist Awaiting requested review from reluctantfuturist reluctantfuturist is a code owner

@mattf mattf Awaiting requested review from mattf mattf is a code owner

@slekkala1 slekkala1 Awaiting requested review from slekkala1 slekkala1 is a code owner

@franciscojavierarceo franciscojavierarceo Awaiting requested review from franciscojavierarceo franciscojavierarceo is a code owner

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cdoern @leseb